-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add support for uninitialized custom params #1898
base: main
Are you sure you want to change the base?
Conversation
63c9279
to
25e1f90
Compare
You probably (is actually highly recommended since it checks for secret leaks and things like that) want to install pre-commit and do a It would save a bit of the planet too by not having wasted CPU cycles :D |
/assign @chmouel |
(prow-commands failes cause it need a rebase with latest change) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can easily add a e2e for this here https://github.com/openshift-pipelines/pipelines-as-code/blob/main/test/gitea_params_test.go#L250-L250
1dbca0b
to
8a53595
Compare
When a Custom Param is defined without any value, it may still be templated-in to the pipeline when a value is provided via a gitops comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to double check manually, since it's kinda hard to read thru the logic.... will let u know when it's done
@@ -45,11 +45,25 @@ spec: | |||
key: companyname | |||
``` | |||
|
|||
Lastly, if no default value makes sense for a custom param, it can be defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if i quite grasp what the "make sense" here mean 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel free to rephrase it and it's good to go for me...
Filled this #1952 (no need to address this part of this pr :) ) |
@chmouel in your example, what should the value have expanded to when you triggered with just |
Changes
Add support for defining Custom Params in the Repository without any value, allowing the value to be supplied by the webhook. Add a new optional field
required
to the Custom Param, which enforces that the value is supplied via the webhook if not specified in the Repository.If a Custom Param is defined without any value, no value is supplied in the payload, and it is not marked as
required
, then the param is omitted and the behaviour has no change from the current state. To this extent the feature is backwards compatible.Documentation is not included yet in chase semantic changes are requested.
Submitter Checklist
📝 Ensure your commit message is clear and informative. Refer to the How to write a git commit message guide. Include the commit message in the PR body rather than linking to an external site (e.g., Jira ticket).
♽ Run make test lint before submitting a PR to avoid unnecessary CI processing. Consider installing pre-commit and running pre-commit install in the repository root for an efficient workflow.
✨ We use linters to maintain clean and consistent code. Run make lint before submitting a PR. Some linters offer a --fix mode, executable with make fix-linters (ensure markdownlint and golangci-lint are installed).
📖 Document any user-facing features or changes in behavior.
🧪 While 100% coverage isn't required, we encourage unit tests for code changes where possible.
🎁 If feasible, add an end-to-end test. See README for details.
🔎 Address any CI test flakiness before merging, or provide a valid reason to bypass it (e.g., token rate limitations).
If adding a provider feature, fill in the following details:
(update the documentation accordingly)